Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get mappings support to high-level rest client #30889

Merged
merged 14 commits into from
Jun 4, 2018

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented May 25, 2018

This adds support for the get mappings API to the high level rest client.

Relates to #27205

This adds support for the get mappings API to the high level rest client.

Relates to elastic#27205
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few minor comments, LGTM otherwise

"_mapping", getMappingsRequest.types()));

Params parameters = new Params(request);
parameters.withMasterTimeout(getMappingsRequest.masterNodeTimeout());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this being set in the corresponding REST action, would you mind opening another PR to fix that? It needs to be added to the SPEC too.


Params parameters = new Params(request);
parameters.withMasterTimeout(getMappingsRequest.masterNodeTimeout());
parameters.withIndicesOptions(getMappingsRequest.indicesOptions());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we need to set the local flag also

@@ -191,6 +192,16 @@ static Request putMapping(PutMappingRequest putMappingRequest) throws IOExceptio
return request;
}

static Request getMappings(GetMappingsRequest getMappingsRequest) throws IOException {
Request request = new Request(HttpGet.METHOD_NAME, endpoint(getMappingsRequest.indices(),
"_mapping", getMappingsRequest.types()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you need to protect both indices and types from NPEs. Unfortunately you can set null from their setter methods. We already do this for other API.

getMappingRequest.indices(indices);

String type = randomAlphaOfLengthBetween(3, 10);
getMappingRequest.types(type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test setting indices and/or types null also

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also test with no types provided?

Map<String, String> expectedParams = new HashMap<>();

setRandomIndicesOptions(getMappingRequest::indicesOptions, getMappingRequest::indicesOptions, expectedParams);
setRandomMasterTimeout(getMappingRequest, expectedParams);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a test around the local flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, was added

return this.mappings.equals(other.mappings);
}

private static final class Fields {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get rid of this inner class and declare the field at the top-level?

@@ -83,6 +84,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
final GetMappingsRequest getMappingsRequest = new GetMappingsRequest();
getMappingsRequest.indices(indices).types(types);
getMappingsRequest.indicesOptions(IndicesOptions.fromRequest(request, getMappingsRequest.indicesOptions()));
getMappingsRequest.masterNodeTimeout(request.paramAsTime("master_timeout", getMappingsRequest.masterNodeTimeout()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh thanks I hadn't noticed that you already added this missing line ++ can you double check the spec too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to the spec

message = String.format(Locale.ROOT, "types [%s] missing", toNamesString(difference.toArray(new String[0])));
}
final String message = String.format(Locale.ROOT, "type" + (difference.size() == 1 ? "" : "s") +
" [%s] missing", Strings.collectionToCommaDelimitedString(difference));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the toNamesString method if we no longer use it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -129,47 +131,15 @@ public RestResponse buildResponse(final GetMappingsResponse response, final XCon
status = RestStatus.OK;
} else {
status = RestStatus.NOT_FOUND;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do think about moving all this logic to the corresponding transport action (as a follow-up)? I didn't look too close but I think that this causes different behaviour between REST and transport at the moment, it would be nice to have a simpler REST action maybe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't love this logic but this isn't the place to remove it, I think it'd be fine as a follow-up. The difficulty is that we are not consistent here (see: #30768 (comment) ) I think we need to sort that out before removing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not talking about removing this (I agree it's not nice and we should talk about that), but rather moving this logic to the transport action. That would maintain the same behaviour at REST and most likely change it for transport client users. Anyway, doesn't belong here.

@@ -83,6 +84,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
final GetMappingsRequest getMappingsRequest = new GetMappingsRequest();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may also want to look at removing RestGetAllMappingsAction if possible. As a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely +1 on removing that if possible.

@dakrone
Copy link
Member Author

dakrone commented May 29, 2018

Thanks for taking a look @javanna, I think I've addressed all of your comments

--------------------------------------------------
<1> Returning all indices' mappings
<2> Retrieving the mappings for a particular index and type
<3> Getting the mappings for the "tweet" as a Java Map
Copy link
Member

@javanna javanna May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get these errors when building the docs:

asciidoc: WARNING: get_mappings.asciidoc: line 14: list item index: expected 2 got 1
asciidoc: WARNING: get_mappings.asciidoc: line 15: list item index: expected 3 got 2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why these occur, I checked and there are 3 annotations in IndicesClientDocumentationIT for this tag, and 3 here, not sure why they are off-by-one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it :)

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides the docs issue, feel free to merge once those are addressed. thanks @dakrone !

@dakrone dakrone merged commit b22a055 into elastic:master Jun 4, 2018
@dakrone
Copy link
Member Author

dakrone commented Jun 4, 2018

Thanks for taking a look @javanna

dnhatn added a commit that referenced this pull request Jun 4, 2018
* master:
  Add get mappings support to high-level rest client (#30889)
  Fix index prefixes to work with span_multi (#31066)
  [DOCS] Removes redundant authorization pages
  [DOCS] Re-adds custom realm
  Change ObjectParser exception (#31030)
  Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (#31073)
@javanna
Copy link
Member

javanna commented Jun 5, 2018

@dakrone do you plan on backporting this as well?

dakrone added a commit that referenced this pull request Jun 5, 2018
This adds support for the get mappings API to the high level rest client.

Relates to #27205
@dakrone dakrone added the v6.4.0 label Jun 5, 2018
jasontedor added a commit that referenced this pull request Jun 5, 2018
* elastic/6.x:
  [Rollup] Disallow index patterns that match the rollup index (#30491)
  Revert "Fixing MixedClusterClientYamlTestSuiteIT"
  Fixing MixedClusterClientYamlTestSuiteIT
  Add get mappings support to high-level rest client (#30889)
  [DOCS] Fixes security example (#31082)
  Allow terms query in _rollup_search (#30973)
@@ -95,6 +95,7 @@ include::indices/clear_cache.asciidoc[]
include::indices/force_merge.asciidoc[]
include::indices/rollover.asciidoc[]
include::indices/put_mapping.asciidoc[]
include::indices/get_mappings.asciidoc[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heya @dakrone this is not enough for the page to be linked, in fact this API is currently missing in our docs, would you mind fixing that please?

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jul 3, 2018
This commit adds the high-level rest client docs for the get mappings API that
was added in elastic#30889
dakrone added a commit that referenced this pull request Jul 3, 2018
This commit adds the high-level rest client docs for the get mappings API that
was added in #30889
dakrone added a commit that referenced this pull request Jul 3, 2018
This commit adds the high-level rest client docs for the get mappings API that
was added in #30889
@dakrone dakrone deleted the hlrc-add-get-mappings-api branch February 4, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants